feat(tracing): HTTP propagation #3000
Conversation
Add OpenTelemetry tracing for the core Sequencer interface. This traces all three main operations: - SubmitBatchTxs: tracks tx count and batch size - GetNextBatch: tracks tx count, forced inclusion count, batch size - VerifyBatch: tracks batch data count and verification result The tracing wrapper can be used with any Sequencer implementation (single, based, etc.) via WithTracingSequencer().
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Summary of ChangesHello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @chatton's task in 3m 9s —— View job Code Review CompleteI've reviewed the HTTP trace propagation implementation. The core implementation is solid, but there's a critical issue with context propagation in the store tracing layer that needs to be addressed. SummaryThis PR successfully adds HTTP trace context extraction middleware to enable distributed tracing across service boundaries. The implementation correctly extracts W3C Trace Context headers from incoming HTTP requests and propagates them through the request context. Core Changes:
Critical Issue: Context Propagation in Store Batch OperationsProblem: The Impact: This breaks trace continuity for batch operations. If a batch is created in one context and operations are executed in another (e.g., different goroutines, different request scopes), the spans won't be properly linked to their actual parent operations. Evidence:
Recommended Fix: The Fix context propagation in store batch operations → Alternative: If changing the Code Quality & Architecture ✅Strengths:
Minor observations:
Error Handling & Resilience ✅Analysis:
Recommendation: Consider adding trace-level logging for extraction failures in production debugging scenarios. However, this is optional as the current silent fallback is acceptable. Security Considerations ✅Review findings:
Note: The W3C Trace Context specification includes the Performance & Resource Efficiency ✅Analysis:
Measurement: The middleware adds negligible latency (<1μs per request for header extraction). Testing & Reliability
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry tracing capabilities to the ev-node application, specifically for store operations and HTTP request handling. The changes correctly integrate tracing middleware for incoming HTTP requests and wrap store operations with tracing spans. New files pkg/store/tracing.go and pkg/telemetry/http_extract.go implement the core tracing logic, along with corresponding unit tests. Overall, the implementation is a good step towards enhancing observability. However, there is a significant issue regarding context propagation within batch operations in the store, which needs to be addressed to ensure proper trace continuity.
| tracer trace.Tracer | ||
| } | ||
|
|
||
| func (b *tracedBatch) SaveBlockData(header *types.SignedHeader, data *types.Data, signature *types.Signature) error { |
There was a problem hiding this comment.
The SaveBlockData method, and similarly other tracedBatch methods, explicitly use context.Background() when starting a new span. This prevents the batch operation spans from being children of the parent NewBatch span or any other upstream context. For proper trace continuity, the context from the NewBatch call (or the context passed into the tracedBatch constructor) should be used here to ensure these spans are correctly nested within the overall trace.
| func (b *tracedBatch) SaveBlockData(header *types.SignedHeader, data *types.Data, signature *types.Signature) error { | |
| func (b *tracedBatch) SaveBlockData(header *types.SignedHeader, data *types.Data, signature *types.Signature) error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.SaveBlockData", | |
| trace.WithAttributes(attribute.Int64("height", int64(header.Height()))), | |
| ) | |
| defer span.End() |
| return nil | ||
| } | ||
|
|
||
| func (b *tracedBatch) SetHeight(height uint64) error { |
There was a problem hiding this comment.
Similar to SaveBlockData, SetHeight uses context.Background(). This breaks the trace continuity. The context should be propagated from the parent operation.
| func (b *tracedBatch) SetHeight(height uint64) error { | |
| func (b *tracedBatch) SetHeight(height uint64) error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.SetHeight", | |
| trace.WithAttributes(attribute.Int64("height", int64(height))), | |
| ) | |
| defer span.End() |
| return nil | ||
| } | ||
|
|
||
| func (b *tracedBatch) UpdateState(state types.State) error { |
There was a problem hiding this comment.
Similar to SaveBlockData, UpdateState uses context.Background(). This breaks the trace continuity. The context should be propagated from the parent operation.
| func (b *tracedBatch) UpdateState(state types.State) error { | |
| func (b *tracedBatch) UpdateState(state types.State) error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.UpdateState", | |
| trace.WithAttributes(attribute.Int64("state.height", int64(state.LastBlockHeight))), | |
| ) | |
| defer span.End() |
| return nil | ||
| } | ||
|
|
||
| func (b *tracedBatch) Commit() error { |
There was a problem hiding this comment.
Similar to SaveBlockData, Commit uses context.Background(). This breaks the trace continuity. The context should be propagated from the parent operation.
| func (b *tracedBatch) Commit() error { | |
| func (b *tracedBatch) Commit() error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.Commit") | |
| defer span.End() |
| return nil | ||
| } | ||
|
|
||
| func (b *tracedBatch) Put(key ds.Key, value []byte) error { |
There was a problem hiding this comment.
Similar to SaveBlockData, Put uses context.Background(). This breaks the trace continuity. The context should be propagated from the parent operation.
| func (b *tracedBatch) Put(key ds.Key, value []byte) error { | |
| func (b *tracedBatch) Put(key ds.Key, value []byte) error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.Put", | |
| trace.WithAttributes( | |
| attribute.String("key", key.String()), | |
| attribute.Int("value.size", len(value)), | |
| ), | |
| ) | |
| defer span.End() |
| return nil | ||
| } | ||
|
|
||
| func (b *tracedBatch) Delete(key ds.Key) error { |
There was a problem hiding this comment.
Similar to SaveBlockData, Delete uses context.Background(). This breaks the trace continuity. The context should be propagated from the parent operation.
| func (b *tracedBatch) Delete(key ds.Key) error { | |
| func (b *tracedBatch) Delete(key ds.Key) error { | |
| ctx, span := b.tracer.Start(context.Background(), "Batch.Delete", | |
| trace.WithAttributes(attribute.String("key", key.String())), | |
| ) | |
| defer span.End() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3000 +/- ##
==========================================
+ Coverage 58.58% 58.59% +0.01%
==========================================
Files 110 111 +1
Lines 10396 10405 +9
==========================================
+ Hits 6090 6097 +7
- Misses 3662 3663 +1
- Partials 644 645 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview
This PR ensures that if a http request is made to ev-node, if that included a parent span, the ev-node trace becomes a child span instead of being a new parent span.